Cortex-M: NVIC and SCB code duplication cleanup #576

agkaminski · 2024-08-09T11:49:53Z

Clenup standard part of Cortex-M component (i.e. SCB and NVIC) mass copy-paste across targets. If this approach passes, then it will be done over plo too

Description

Motivation and Context

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)
Chore (refactoring, style fixes, git/CI config, submodule management, no code logic changes)

How Has This Been Tested?

Already covered by automatic testing.
New test added: (add PR link here).
Tested by hand on: (mcxn94x).

Checklist:

My change requires a change to the documentation.
I have updated the documentation accordingly.
I have added tests to cover my changes.
All new and existing linter checks and tests passed.
My changes generate no new compilation warnings for any of the targets.

Special treatment

This PR needs additional PRs to work (list the PRs, preferably in merge-order).
I will merge this PR by myself when appropriate.

hal/arm/nvic.c

hal/arm/Makefile

hal/arm/nvic.c

hal/arm/nvic.h

hal/arm/scb.c

github-actions · 2024-08-09T12:45:07Z

Unit Test Results

7 700 tests ±0 6 985 ✅ ±0 39m 14s ⏱️ - 1m 5s
436 suites ±0 715 💤 ±0
1 files ±0 0 ❌ ±0

Results for commit 4762279. ± Comparison against base commit bcc5c41.

♻️ This comment has been updated with latest results.

hal/arm/scb.c

hal/arm/nvic.c

JIRA: RTOS-886

agkaminski · 2024-09-13T14:01:58Z

@anglov just to verify if it does not break your stuff

anglov · 2024-09-19T16:20:05Z

I really like this change as it remove a lot of duplicated code. I check it on DEND (checked low power) and NIL projects and at the first glance it looks ok!

Some general remarks (or ideas for future):

I don't like using "SCB" and "System Control Block" name
According to Armv7-M Architecture Reference Manual (DDI0403E.e) and Armv8-M Architecture Reference Manual (DDI0553B.y) SCB is small part of System Control Space which also contain NVIC, Systick, MPU and other control, debug and id registers.
It's confusing for the first time.
Maybe more appropriate name should be "SCS", as for "System Control Space"?
Maybe NVIC registers should also be used from one big SCS structure?
I see that interrupts.c on armv7m architectures differs mainly with includes and priority of interrupts. It probably should be revised and also use common implementation in the future.
It's worth considering if _init.S couldn't be shared between cores somehow.
armv8m implementations differs mainly with interrupt size, hal entry point and cosmetic things and approaches if I didn't miss anything.
In armv7m implementation it differs mainly with interrupt size FPU hard/soft implementation and .cpu directive. In cortex-m4 was also errata requiring additional dsb at intertupt dispatch, which was propagated to cortex-m7 too anyways.

Current refactoring is great step in good direction anyways.

Darchiv

Huge improvement, nice! Just two remarks (for the future, PR can be merged as-is):

Check cache availability in _hal_scbCacheIsSupported() via Cache Control Identification Registers instead of relying solely on cpuid->PARTNO, because cache is a silicon option.
A few functions are missing, e.g. _imxrt_nvicGetPendingIRQ(). They were not used, but maybe they should not be scrapped if code already exists.